⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 71.13%. Comparing base (6bc90a8) to head (67a8593).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
- Coverage   71.16%   71.13%   -0.04%     
==========================================
  Files          26       26              
  Lines        3114     3114              
  Branches      527      528       +1     
==========================================
- Hits         2216     2215       -1     
- Misses        766      767       +1     
  Partials      132      132              

FTR, the .save() method could benefit from some refactoring. Nothing critical, though.

Hm. I'm not sure ignoring the errors like this is the right thing to do. After all, the mappings will then be out of sync. If the device doesn't support dynamic re-mapping, then calling .save() isn't the right thing to do, but rather .read(). A better approach to catch this in advance would be to look at the AccessType attribute (via .writable property) in the Object Dictionary before even trying to save, or at least while saving, before attempting the actual SDO access. If the OD is wrong on that account, then fix your EDS :-)

FTR, the .save() method could benefit from some refactoring. Nothing critical, though.

What do you have in mind exactly? Sure, it ain't pretty, but I wouldn't want to introduce lots of internal methods without a clear benefit (like reusability).

Hm. I'm not sure ignoring the errors like this is the right thing to do. After all, the mappings will then be out of sync. If the device doesn't support dynamic re-mapping, then calling .save() isn't the right thing to do, but rather .read(). A better approach to catch this in advance would be to look at the AccessType attribute (via .writable property) in the Object Dictionary before even trying to save, or at least while saving, before attempting the actual SDO access. If the OD is wrong on that account, then fix your EDS :-)

Thanks for your insight; yes, using the the OD is a better approach. For my case, I see that the supplied EDS file has subindices of 1A00 marked as read-only. I'll rewrite the PR asap.

What do you have in mind exactly? Sure, it ain't pretty, but I wouldn't want to introduce lots of internal methods without a clear benefit (like reusability).

Nothing in mind; just a drive-by observation :)

Just to be clear about the motivation for this change. You're trying to save the PDO settings because you changed something besides the mapping parameters. The communication parameters are writable, but mappings are not. Therefore you want to save only the writable parameters.

I wonder why we should do this only for the mappings. If some other setting is read-only, it may benefit from the same treatment. And I don't feel really comfortable with just silently skipping the entries. There should be some logging at least to indicate the operation was only partially carried out.

This still leaves the problem that the read-only settings are then likely out of sync, because if we couldn't write the value, we don't read it back either. So in this case, a read operation is in order after the partial saving, to sync the read-only entries. Or they must be initialized correctly from the OD first with the recorded constant value.

Just to be clear about the motivation for this change. You're trying to save the PDO settings because you changed something besides the mapping parameters. The communication parameters are writable, but mappings are not. Therefore you want to save only the writable parameters.

Correct; specifically in my case, I want to disable some TPDOs and modify the SYNC behaviour of others.

I wonder why we should do this only for the mappings. If some other setting is read-only, it may benefit from the same treatment.

That makes sense.

And I don't feel really comfortable with just silently skipping the entries. There should be some logging at least to indicate the operation was only partially carried out.

I wondered about this. In this case I don't think it will be too annoying with more, rather than less, logging. I guess INFO would be an appropriate level if the values are not changed, and WARNING if a local configuration change was made and scheduled for download. (Can we even detect this, though? I did not look yet.)

This still leaves the problem that the read-only settings are then likely out of sync, because if we couldn't write the value, we don't read it back either. So in this case, a read operation is in order after the partial saving, to sync the read-only entries. Or they must be initialized correctly from the OD first with the recorded constant value.

How about raising an exception if the user tries to write to a read-only variable, so we'd prevent this before we reach save()?

node.tpdo.read()
node.tpdo[1].trans_type = 0  # BOOM! Trying to set a read-only field
node.tpdo.save()

I guess INFO would be an appropriate level if the values are not changed, and WARNING if a local configuration change was made and scheduled for download. (Can we even detect this, though? I did not look yet.)

That's precisely the problem. We cannot know without either uploading or downloading the objects. The correct approach IMHO would be to read the settings, modify some, write back and then it's okay for some updates to be skipped.

How about raising an exception if the user tries to write to a read-only variable, so we'd prevent this before we reach save()?

That's a nice idea, but not always possible. First, assigning the variable happens when the user does it, but also when the read method gets the uploaded value. And then there might be cases where the PDO configuration is done "offline": You know the device's settings and just configure the library to have the same picture, without actually asking the device. That saves a whole bunch of SDO exchanges and wouldn't work well if assigning the attribute is blocked just because the hypothetical SDO write access would fail.

That's a nice idea, but not always possible. First, assigning the variable happens when the user does it, but also when the read method gets the uploaded value.

We can solve the public API using properties if the OD provides what's needed to determine writability.

That doesn't solve the second issue with the approach though. I think it complicates things. Currently some use cases may run into an exception, but we shouldn't prevent other use cases while fixing that. The only ways to know the library state reflects the device configuration is to either read or write PDO config. Splitting up the writing into communication and mapping parameters (e.g. via optional boolean skip_* parameters or separate methods) may be a solution for your specific case. I don't see us restricting the API use though, based on some assumptions about a value being "changed". Downloading (saving) a read-only parameter is simply the wrong thing to do and should be skipped. And to see whether it was changed from some other (which?) value, you need to do the right thing: upload (read) it.

Perhaps this is best solved as a documentation update. It seems pleasing all use cases is too much hassle for the win.

Logging failed entries instead of raising an SDO exception is still an improvement. If OD says it's not writable, then log at INFO level and continue with the next entry / block. That should get noticed during development. I'd prefer it being added for all parameters though, instead of just individual mapping entries.

The documentation update should then advise what to do if this type of INFO message appears:

• The device only partially supports changing PDO configuration. To make sure the library's view of the PDO configuration is in sync with the device, try calling .read() first, then modify the entries which were not mentioned as being skipped (the others cannot be changed). Finally, after calling .save() all entries should match up. If the device actually does changing an entry, make sure the Object Dictionary description correctly reflects that write access capability.

Something along those lines...